-
Notifications
You must be signed in to change notification settings - Fork 416
Update channel_reestablish
for splicing
#3886
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
06c0dfc
to
f000b76
Compare
@wpaulino Ready for review now. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3886 +/- ##
==========================================
+ Coverage 88.61% 88.72% +0.10%
==========================================
Files 174 176 +2
Lines 127640 128918 +1278
Branches 127640 128918 +1278
==========================================
+ Hits 113113 114381 +1268
+ Misses 12046 11942 -104
- Partials 2481 2595 +114
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
🔔 3rd Reminder Hey @wpaulino! This PR has been waiting for your review. |
🔔 4th Reminder Hey @wpaulino! This PR has been waiting for your review. |
🔔 5th Reminder Hey @wpaulino! This PR has been waiting for your review. |
🔔 6th Reminder Hey @wpaulino! This PR has been waiting for your review. |
f000b76
to
e2ea3bf
Compare
Rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to double check our incoming channel_ready
handling to make sure we can handle receiving one long after channel_ready
was already exchanged due to the new logic surrounding your_last_funding_locked_txid
.
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
e2ea3bf
to
69be701
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to double check our incoming
channel_ready
handling to make sure we can handle receiving one long afterchannel_ready
was already exchanged due to the new logic surroundingyour_last_funding_locked_txid
.
ACK. Also need to address https://github.com/lightningdevkit/rust-lightning/pull/3736/files#r2133028859.
f7b9785
to
b0291f4
Compare
Latest push adds some commits to address this. |
Looks like we have some logic below. I think the catch-all case still holds? We should only receive a rust-lightning/lightning/src/ln/channel.rs Lines 6491 to 6519 in 42085b9
|
While splicing is not yet fully supported, checking if the feature has been negotiated is needed for changes to the channel_reestablish logic.
The splicing spec extends the channel_reestablish message with two more TLVs indicating which funding txid the sender has sent/received either explicitly via splice_locked or implicitly via channel_ready. This allows peers to detect if a splice_locked was lost during disconnection and must be retransmitted. This commit updates channel_reestablish with the TLVs. Subsequent commits will implement the spec requirements.
The previous commit extended the channel_reestablish message with your_last_funding_locked_txid and my_current_funding_locked_txid for use as described there. This commit sets those fields to the funding txid most recently sent/received accordingly.
f5c9271
to
719ade4
Compare
Done. |
lightning/src/ln/channel.rs
Outdated
@@ -1215,6 +1215,7 @@ pub(super) struct ReestablishResponses { | |||
pub shutdown_msg: Option<msgs::Shutdown>, | |||
pub tx_signatures: Option<msgs::TxSignatures>, | |||
pub tx_abort: Option<msgs::TxAbort>, | |||
pub implicit_splice_locked: Option<msgs::SpliceLocked>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bleh, let's please note rely on ChannelManager
to pipe channel.rs
logic around in a loop. We need to be exposing Channel
-internal to ChannelManager
less, not more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May just need to rename this as there is some update logic that needs to be executed in ChannelManager::internal_splice_locked
. Will re-visit to see if there's a better alternative.
lightning/src/ln/channel.rs
Outdated
} else { | ||
session.holder_tx_signatures().clone() | ||
} | ||
if !session.has_received_commitment_signed() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this unreachable per the comment in maybe_get_next_funding_txid
- "Since we have a signing_session, this implies we've sent an initial commitment_signed
"? Or do we switch to interactive signing once we send our CS, before our peer sends their CS (and should we?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do we switch to interactive signing once we send our CS, before our peer sends their CS (and should we?).
Based on offline discussion with @wpaulino this statement is true.
lightning/src/ln/channel.rs
Outdated
// - MUST retransmit its `commitment_signed` for that funding transaction. | ||
if msg.next_local_commitment_number == next_counterparty_commitment_number { | ||
// `next_counterparty_commitment_number` is guaranteed to always be the | ||
// commitment number of the `commitment_signed` message we sent for this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong re: #4014.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wpaulino said he will take a look when reviewing. Seems we'll need to follow-up in the spec meeting to potentially add a retransmit flag to retransmit commitment_signed
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could either of you comment on the spec PR as to what you had in mind with retransmit_flags
? Might be worth having t-bast think on it before the spec meeting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to next_funding_txid
to be next_funding
with a txid
and retransmit_flags
as discussed on the spec.
lightning/src/ln/channel.rs
Outdated
// - if it has already received `tx_signatures` for that funding transaction: | ||
// - MUST send its `tx_signatures` for that funding transaction. | ||
if (session.has_received_commitment_signed() && session.holder_sends_tx_signatures_first()) | ||
|| self.context.channel_state.is_their_tx_signatures_sent() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to check has_received_commitment_signed()
even in the case where they already sent their tx signatures? Or can the state machine not advance in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on offline discussion self.context.channel_state.is_their_tx_signatures_sent()
implies session.has_received_commitment_signed()
.
lightning/src/ln/channel.rs
Outdated
// on reestablish and tell our peer to just forget about it. | ||
// Our peer is doing something strange, but it doesn't warrant closing the channel. | ||
(None, None, Some(msgs::TxAbort { | ||
// The `next_funding_txid` does not match the latest interactive funding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the else
block for where maybe_get_next_funding_txid
returned None
, not where the txid doesn't match, so the comment is confusing. But I'm also a bit less clear on the second case of maybe_get_next_funding_txid
- if we have an interactive signing session, but they did send their tx signatures (and we received them), we'll end up here, but it seems its much too late to TxAbort
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From offline discussion we'll drop the else
block and use of maybe_get_next_funding_txid
in the if
block. Instead, we'll get the latter from the signing session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
}) | ||
} else if msg.next_local_commitment_number == next_counterparty_commitment_number - 1 { | ||
// We've made an update so we must have exchanged `tx_signatures`, implying that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might indicate we're sending our CS for the interactive signing session, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to #3886 (comment), so will depend on how that is resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed spec change did not include a flag for re-transmitting tx_signatures
, so kept this as is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's because the next_funding
TLV being set implies that. Now that next_counterparty_commitment_number
is no longer affected by splicing, we shouldn't be re-transmitting a tx_signatures
when they're behind by one state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you missed this, but this comment needs to change because it shouldn't be possible to retransmit tx_signatures
in this case (and we should assert it's None
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed recent review comments offline. Left some notes about what was decided. @TheBlueMatt @wpaulino Feel free to expand on anything there that isn't clear. I'll address anything that can be resolved and not blocked on discussion at the spec meeting.
lightning/src/ln/channel.rs
Outdated
} else { | ||
session.holder_tx_signatures().clone() | ||
} | ||
if !session.has_received_commitment_signed() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do we switch to interactive signing once we send our CS, before our peer sends their CS (and should we?).
Based on offline discussion with @wpaulino this statement is true.
lightning/src/ln/channel.rs
Outdated
// - MUST retransmit its `commitment_signed` for that funding transaction. | ||
if msg.next_local_commitment_number == next_counterparty_commitment_number { | ||
// `next_counterparty_commitment_number` is guaranteed to always be the | ||
// commitment number of the `commitment_signed` message we sent for this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wpaulino said he will take a look when reviewing. Seems we'll need to follow-up in the spec meeting to potentially add a retransmit flag to retransmit commitment_signed
.
lightning/src/ln/channel.rs
Outdated
// - if it has already received `tx_signatures` for that funding transaction: | ||
// - MUST send its `tx_signatures` for that funding transaction. | ||
if (session.has_received_commitment_signed() && session.holder_sends_tx_signatures_first()) | ||
|| self.context.channel_state.is_their_tx_signatures_sent() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on offline discussion self.context.channel_state.is_their_tx_signatures_sent()
implies session.has_received_commitment_signed()
.
lightning/src/ln/channel.rs
Outdated
// on reestablish and tell our peer to just forget about it. | ||
// Our peer is doing something strange, but it doesn't warrant closing the channel. | ||
(None, None, Some(msgs::TxAbort { | ||
// The `next_funding_txid` does not match the latest interactive funding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From offline discussion we'll drop the else
block and use of maybe_get_next_funding_txid
in the if
block. Instead, we'll get the latter from the signing session.
}) | ||
} else if msg.next_local_commitment_number == next_counterparty_commitment_number - 1 { | ||
// We've made an update so we must have exchanged `tx_signatures`, implying that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to #3886 (comment), so will depend on how that is resolved.
lightning/src/ln/channel.rs
Outdated
@@ -1215,6 +1215,7 @@ pub(super) struct ReestablishResponses { | |||
pub shutdown_msg: Option<msgs::Shutdown>, | |||
pub tx_signatures: Option<msgs::TxSignatures>, | |||
pub tx_abort: Option<msgs::TxAbort>, | |||
pub implicit_splice_locked: Option<msgs::SpliceLocked>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May just need to rename this as there is some update logic that needs to be executed in ChannelManager::internal_splice_locked
. Will re-visit to see if there's a better alternative.
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
719ade4
to
7c948e4
Compare
🔔 3rd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
7c948e4
to
6850814
Compare
Squashed as requested offline. |
6850814
to
5feb4fd
Compare
Pushed a |
@@ -8665,9 +8665,9 @@ where | |||
} | |||
|
|||
if msg.next_local_commitment_number >= INITIAL_COMMITMENT_NUMBER || msg.next_remote_commitment_number >= INITIAL_COMMITMENT_NUMBER || | |||
(msg.next_local_commitment_number == 0 && msg.next_funding_txid.is_none()) { | |||
(msg.next_local_commitment_number == 0 && msg.next_funding.is_none()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change can be dropped now that the next_local_commitment_number
isn't ever decremented in the next_funding
case, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which change? This line is just updating the field name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, the msg.next_funding.is_none
check can be dropped though since it cannot be zero anymore if next_funding
is set.
lightning/src/ln/channel.rs
Outdated
// `next_counterparty_commitment_number` is guaranteed to always be the | ||
// commitment number of the `commitment_signed` message we sent for this | ||
// funding transaction. If they set `next_funding`, then they should not have | ||
// processed our `tx_signatures` yet, which implies that our state machine is | ||
// still paused and no updates can happen that would increment our | ||
// `next_counterparty_commitment_number`. | ||
// | ||
// If they did set `next_funding` even after processing our `tx_signatures` | ||
// erroneously, this may end up resulting in a force close. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this whole comment isn't really relevant anymore
}) | ||
} else if msg.next_local_commitment_number == next_counterparty_commitment_number - 1 { | ||
// We've made an update so we must have exchanged `tx_signatures`, implying that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's because the next_funding
TLV being set implies that. Now that next_counterparty_commitment_number
is no longer affected by splicing, we shouldn't be re-transmitting a tx_signatures
when they're behind by one state.
lightning/src/ln/channel.rs
Outdated
@@ -10492,6 +10492,25 @@ where | |||
self.sign_channel_announcement(node_signer, announcement).ok() | |||
} | |||
|
|||
fn get_next_local_commitment_number(&self) -> u64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this commit can be dropped now that we use the flag to signal commitment_signed
needs to be retransmitted?
lightning/src/ln/channel.rs
Outdated
// - MUST set the `announcement_signatures` bit to `1` in `retransmit_flags`. | ||
// - otherwise: | ||
// - MUST set the `announcement_signatures` bit to `0` in `retransmit_flags`. | ||
if self.funding.get_funding_txid() == Some(txid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we still mark that the sigs need to be sent when we haven't promoted the scope yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right since they could have sent both splice_locked
and announcement_signatures
but we didn't receive either. So they would have promoted the splice but we hadn't yet. We'd infer their splice_locked
on reconnect but would need to request signatures.
@@ -8744,6 +8761,7 @@ where | |||
shutdown_msg, announcement_sigs, | |||
tx_signatures: None, | |||
tx_abort: None, | |||
inferred_splice_locked: None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it the case that we can be in AwaitingChannelReady
but the peer needs a splicing signature (ie we sent ours second and they didn't receive it)? In that case we would get and return early but we really need to send a response message. I guess that's a dual-funding question not a splicing question, but might be worth leaving a TODO here or whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not understanding the scenario described here.
Isn't it the case that we can be in
AwaitingChannelReady
but the peer needs a splicing signature (ie we sent ours second and they didn't receive it)?
Sent a splice_locked
second? I don't think we ever transition to AwaitingChannelReady
during splicing.
In that case we would get and return early but we really need to send a response message.
Which message?
I guess that's a dual-funding question not a splicing question, but might be worth leaving a TODO here or whatever.
Hmm? Are you saying we should send a channel_ready
?
lightning/src/ln/channel.rs
Outdated
// | ||
// If they did set `next_funding` even after processing our `tx_signatures` | ||
// erroneously, this may end up resulting in a force close. | ||
let commitment_signed = self.context.get_initial_commitment_signed_v2(&self.funding, logger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this need to use the funding scope of the current splice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, though if there isn't one it needs to be &self.funding
as it indicates the signing session is for the initial funding, IIUC.
} | ||
|
||
// TODO(splicing): Add comment for spec requirements | ||
if next_funding.should_retransmit(msgs::NextFundingFlag::CommitmentSigned) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we may generate both a commitment_signed
and a tx_signatures
but the commitment_signed
needs to go first. Sadly, ChannelManager::handle_channel_resumption
sends tx_signatures
first so that needs to swap.
Phew that simplified this a lot. |
During channel reestablishment, both commitment_signed and tx_signatures messages may be retransmitted. However, commitment_signed must come first, so re-arrange the ordering. This moves tx_abort as well to keep the interactive tx messages together.
The splicing spec updates the logic pertaining to next_funding_txid when handling a channel_reestablish message. Specifically: A receiving node: - if `next_funding_txid` is set: - if `next_funding_txid` matches the latest interactive funding transaction or the current channel funding transaction: - if `next_commitment_number` is equal to the commitment number of the `commitment_signed` message it sent for this funding transaction: - MUST retransmit its `commitment_signed` for that funding transaction. - if it has already received `commitment_signed` and it should sign first, as specified in the [`tx_signatures` requirements](#the-tx_signatures-message): - MUST send its `tx_signatures` for that funding transaction. - if it has already received `tx_signatures` for that funding transaction: - MUST send its `tx_signatures` for that funding transaction. - if it also sets `next_funding_txid` in its own `channel_reestablish`, but the values don't match: - MUST send an `error` and fail the channel. - otherwise: - MUST send `tx_abort` to let the sending node know that they can forget this funding transaction. Note that the spec is in flux. Instead, next_funding_txid is replaced with next_funding, which contains both a txid and retransmit_flags. The latter is used instead of next_commitment_number to determine whether commitment_signed should be retransmitted. This commit updates FundedChannel::channel_reestablish accordingly. Co-authored-by: Wilmer Paulino <wilmer@wilmerpaulino.com> Co-authored-by: Jeffrey Czyz <jkczyz@gmail.com>
When a splice transaction is promoted (i.e., when splice_locked has been exchanged), announcement_signatures must be sent. However, if we try to send a channel_announcement before they are received, then the signatures will be incorrect. To avoid this, clear the counterparty's announcement_signatures upon promoting a FundingScope.
When handling a counterparties channel_reestablish, the spec dictates that a splice_locked may be implied by my_current_funding_locked. Compare that against any pending splices and handle an implicit splice_locked message when applicable.
During channel reestablishment, announcement_signatures may need to be retransmitted. The splicing spec allows doing so without retransmitting splice_locked first, which could normally trigger retransmitting announcement_signatures. Instead, my_current_funding_locked lets the sender request retransmitting it.
The previous commit allowed requesting retransmission of announcement_signatures during channel reestablishment. This commit handles such requests.
5feb4fd
to
6e3c071
Compare
} | ||
(commitment_update, tx_signatures, None) | ||
}) | ||
.or_else(|| Some(&self.funding)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a case where this is possible. If next_funding
is set then they haven't processed our tx_signatures
, so the funding cannot be locked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh never mind, I see this is to handle the dual funding case.
The splicing spec extends the
channel_reestablish
message with two more TLVs indicating which funding txid the sender has sent/received either explicitly viasplice_locked
or implicitly viachannel_ready
. This allows peers to detect if asplice_locked
was lost during disconnection and must be retransmitted.To this end, the spec updates the
channel_reestablish
logic to support splicing.